Adds `within` and `eval_code` to `teal_data_module` #983

averissimo · 2023-11-28T11:22:00Z

Pull Request

Fixes #nnn

Changes description

Adds within and eval_code to teal_data_module
Adds tests for new functions (100% coverage)
Updates data-as-shiny-module vignette
🟠 Moves {teal.code} from Suggests to Imports

averissimo · 2023-11-28T11:33:44Z

R/teal_data_module-eval_code.R

+within.teal_data_module <- function(data, expr, ...) {
+  expr <- substitute(expr)
+  extras <- list(...)
+
+  # Add braces for consistency.
+  if (!identical(as.list(expr)[[1L]], as.symbol("{"))) {
+    expr <- call("{", expr)
+  }
+
+  calls <- as.list(expr)[-1]
+
+  # Inject extra values into expressions.
+  calls <- lapply(calls, function(x) do.call(substitute, list(x, env = extras)))
+
+  eval_code(object = data, code = as.expression(calls))
+}


The body is a copy from teal.code:::within.qenv, it just prepares the arguments to call eval_code() (data is unchanged)

So in practice it could be replaced with code below (reducing duplicated code accross {teal.*}):

Thoughts?

Suggested change

within.teal_data_module <- function(data, expr, ...) {

expr <- substitute(expr)

extras <- list(...)

# Add braces for consistency.

if (!identical(as.list(expr)[[1L]], as.symbol("{"))) {

expr <- call("{", expr)

}

calls <- as.list(expr)[-1]

# Inject extra values into expressions.

calls <- lapply(calls, function(x) do.call(substitute, list(x, env = extras)))

eval_code(object = data, code = as.expression(calls))

}

within.teal_data_module <- function(data, expr, ...) {

within.qenv <- getFromNamespace("within.qenv", "teal.code")

within.qenv(data, expr, ...)

}

I think getS3method("within", "qenv", envir = as.environment(getNamespace("teal.code"))) would be a better choice.

It shouldn't be necessary anyway because teal.code must be installed when teal is.

At the moment within only processes the expression and calls eval_code. In this context this is a valid simplification. I'm not sure, though 🤔

I think getS3method("within", "qenv", envir = as.environment(getNamespace("teal.code"))) would be a better choice.

It shouldn't be necessary anyway because teal.code must be installed when teal is.

That's better! Is the envir parameter required? (as you said, teal.code is imported and it seems to find the within)

At the moment within only processes the expression and calls eval_code. In this context this is a valid simplification. I'm not sure, though 🤔

@vedhav brough a smilar concern, I'm on the fence as I prefer to minimize repetitive code, but I don't think this solution is "best practice" (calling within.qenv with a different data object seems like a red flag).

I would prefer if there was within.default (non-exported) or .generic_within in {teal.code} that was the backbone of both of those calls and still allow for deviations in the future of within.qenv that wouldn't affect within.teal_data_module.

That's better! Is the envir parameter required?

It isn't but just in case there is a different within.qenv defined somewhere...

I would prefer if there was within.default

The within generic is a base function so defining a default that suits our particular classes would cause a mess.

The within generic is a base function so defining a default that suits our particular classes would cause a mess.

Yup, so a .generic_within function would be best if we wanted to go this route.

Using the .default was a suggestion if it didn't interfere with base::within (by not exporting it, but even then it might be "dangerous")

Ah, but S3 methods must be exported 😉

@gogonzo would you like to pitch in?

One interesting pattern that I have found when working on the verdepcheck is to make a simple (i.e. non S3 / non S4 etc.) (implementation) function for each method and these methods are essentially a one-liner calling those functions. Please find an example in R6 context: https://github.com/r-lib/pkgdepends/blob/main/R/pkg-plan.R - all public methods are calling private (implementation) functions.

Then you can re-use those implementation functions from multiple classes but there are some obvious drawbacks though such as number of objects in a package, documentation, maintenance, debugging etc.

This however is not addressing an exporting / importing issue mentioned here 😈

github-actions · 2023-11-28T13:17:11Z

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  92      50  45.65%   9-71
R/get_rcode_utils.R                  38       1  97.37%   50
R/include_css_js.R                   24       0  100.00%
R/init.R                             79      25  68.35%   139, 164-185, 217-219, 221-223, 225-226
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_filter_manager.R           107      29  72.90%   62-70, 79-84, 228, 233-246
R/module_nested_tabs.R              174      14  91.95%   72, 119, 138-145, 163, 216, 238, 271
R/module_snapshot_manager.R         209     157  24.88%   87-99, 127-136, 140-152, 154-161, 168-182, 186-188, 190-195, 198-208, 211-227, 236-251, 265-288, 291-302, 305-311, 325, 346-369
R/module_tabs_with_filters.R         73       2  97.26%   95, 140
R/module_teal_with_splash.R         120      11  90.83%   73, 87-95, 122
R/module_teal.R                     141       8  94.33%   68, 71, 158-159, 165, 196, 204-205
R/modules_debugging.R                18      18  0.00%    25-44
R/modules.R                         143      26  81.82%   119, 132, 226-229, 243-248, 259-263, 378-421
R/reporter_previewer_module.R        18       2  88.89%   26, 30
R/show_rcode_modal.R                 20      20  0.00%    16-37
R/tdata.R                            39       1  97.44%   158
R/teal_data_module-eval_code.R       34       0  100.00%
R/teal_data_module.R                  6       0  100.00%
R/teal_reporter.R                    60       5  91.67%   65, 116-117, 120, 137
R/teal_slices-store.R                25       0  100.00%
R/teal_slices.R                      59      12  79.66%   135-148
R/utils.R                           108      27  75.00%   113-140
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   109-371
R/zzz.R                              11       7  36.36%   3-14
TOTAL                              1713     477  72.15%

Diff against main

Filename                          Stmts    Miss  Cover
------------------------------  -------  ------  --------
R/teal_data_module-eval_code.R      +34       0  +100.00%
TOTAL                               +34       0  +0.56%

Results for commit: b1625ba

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2023-11-28T13:18:04Z

Unit Tests Summary

    1 files   19 suites 12s ⏱️
207 tests 207 ✔️ 0 💤 0 ❌
403 runs 403 ✔️ 0 💤 0 ❌

Results for commit b1625ba.

♻️ This comment has been updated with latest results.

github-actions · 2023-11-28T13:18:08Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
teal_data_module-eval_code	👶		$+0.11$	$+16$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	$±Time$	Test Case
module_teal_with_splash	👶	$+0.03$	srv_teal_with_splash_accepts_data_after_within.teal_data_module
module_teal_with_splash	👶	$+0.04$	srv_teal_with_splash_throws_error_when_within.teal_data_module_returns_NULL
module_teal_with_splash	👶	$+0.04$	srv_teal_with_splash_throws_error_when_within.teal_data_module_returns_arbitrary_object_other_than_teal_data_or_qenv.error_
module_teal_with_splash	👶	$+0.04$	srv_teal_with_splash_throws_error_when_within.teal_data_module_returns_qenv.error
teal_data_module-eval_code	👶	$+0.02$	eval_code.teal_data_module_handles_a_NULL_result
teal_data_module-eval_code	👶	$+0.01$	eval_code.teal_data_module_handles_an_arbitrary_object_other_than_teal_data_or_qenv.error_
teal_data_module-eval_code	👶	$+0.01$	eval_code.teal_data_module_propagates_qenv_error_from_the_original_first_call
teal_data_module-eval_code	👶	$+0.01$	eval_code.teal_data_module_throws_error_when_original_teal_data_module_result_is_not_reactive
teal_data_module-eval_code	👶	$+0.01$	eval_code.teal_data_module_ui_has_modified_namespace_for_id
teal_data_module-eval_code	👶	$+0.02$	eval_code.teal_data_module_will_execute_several_times_until_error
teal_data_module-eval_code	👶	$+0.02$	within.teal_data_module_modifies_the_reactive_tea_data_object
teal_data_module-eval_code	👶	$+0.01$	within.teal_data_module_returns_an_object_with_teal_data_module_class

Results for commit 9163527

♻️ This comment has been updated with latest results.

R/zzz.R

gogonzo

There are missing tests and poc for teal application. There are already tests on data (teal_data_module) output. I wonder what would be the output of eval_code(teal_data_module, code) if there is an error in teal_data_module or in the code.

tests/testthat/test-teal_data_module-eval_code.R

R/teal_data_module-eval_code.R

see insightsengineering/teal.code#176

R/zzz.R

gogonzo

Nothing to add in my opinion. Good job!

R/teal_data_module-eval_code.R

chlebowa · 2023-12-04T13:30:07Z

R/teal_data_module-eval_code.R

+within.teal_data_module <- function(data, expr, ...) {
+  expr <- substitute(expr)
+  extras <- list(...)
+
+  # Add braces for consistency.
+  if (!identical(as.list(expr)[[1L]], as.symbol("{"))) {
+    expr <- call("{", expr)
+  }
+
+  calls <- as.list(expr)[-1]
+
+  # Inject extra values into expressions.
+  calls <- lapply(calls, function(x) do.call(substitute, list(x, env = extras)))
+
+  eval_code(object = data, code = as.expression(calls))
+}


@gogonzo would you like to pitch in?

chlebowa · 2023-12-04T14:48:37Z

R/teal_data_module-eval_code.R

+                    sep = "\n",
+                    "Error when executing `teal_data_module`:",
+                    paste0(
+                      "It must always return a reactive with `teal_data`, it returns object of class(es): ",


As in srv_teal_with_splash, I believe this error message is for the app dev, not for the app user. It should shop up in the console, not in the app.

It depends. Let's say we have teal_data_module that reads csv files only and an app user select e.g. pdf file then we probably need to expose error message for app user.

Then it is up to the module developer to validate the file choice.

R/teal_data_module-eval_code.R

Co-authored-by: Aleksander Chlebowski <[email protected]> Signed-off-by: André Veríssimo <[email protected]>

gogonzo

Please address comments and fix R CMD check

tests/testthat/test-teal_data_module-eval_code.R

R/teal_data_module-eval_code.R

averissimo added 2 commits November 28, 2023 12:15

quick prototype for eval_code and teal

03d5b7e

feat: adds documentation and tests

dc6739f

averissimo added enhancement New feature or request core labels Nov 28, 2023

remove code attribute

293f25a

averissimo commented Nov 28, 2023

View reviewed changes

averissimo added 2 commits November 28, 2023 12:52

docs: corrects documentation to pass checks

3a8579d

checks: corrects note with methods

c04eb56

averissimo marked this pull request as ready for review November 28, 2023 13:13

averissimo added 5 commits November 28, 2023 14:21

lintr: ignores name linter and implements suggestion

2992271

docs: updates pkgdown

f65a389

fix: corrects example

d8a095e

fix: use event_reactive to avoid problems when data is NULL

417c2ee

docs: update teal_data_module vignette

9605f05

averissimo commented Nov 28, 2023

View reviewed changes

R/zzz.R Outdated Show resolved Hide resolved

averissimo and others added 2 commits November 28, 2023 16:30

tests: minor changes

fa6ef28

Merge branch 'main' into ddl@main

441652d

gogonzo reviewed Nov 30, 2023

View reviewed changes

tests/testthat/test-teal_data_module-eval_code.R Outdated Show resolved Hide resolved

tests/testthat/test-teal_data_module-eval_code.R Outdated Show resolved Hide resolved

R/teal_data_module-eval_code.R Outdated Show resolved Hide resolved

averissimo added 8 commits December 1, 2023 13:53

fix: only performs eval_code if teal_data

8bf8aaa

tests: implement @gogonzo suggestion about indirect tests

51dcadf

fix: eval_code only accepts teal_data or propagates error messages

da3c100

tests: rename test

c15c106

fix: swaps validate with qenv.error

c2f708a

tests: using quote on an eval_code test

34db6c6

fix: typo and removed repeated test

ef1877c

fix: using lang2calls

a861bec

see insightsengineering/teal.code#176

pawelru reviewed Dec 1, 2023

View reviewed changes

R/zzz.R Show resolved Hide resolved

gogonzo self-assigned this Dec 4, 2023

chlebowa self-assigned this Dec 4, 2023

gogonzo approved these changes Dec 4, 2023

View reviewed changes

R/teal_data_module-eval_code.R Outdated Show resolved Hide resolved

R/teal_data_module-eval_code.R Outdated Show resolved Hide resolved

R/teal_data_module-eval_code.R Outdated Show resolved Hide resolved

chlebowa reviewed Dec 4, 2023

View reviewed changes

averissimo and others added 5 commits December 4, 2023 16:56

Update R/teal_data_module-eval_code.R

e31ae85

Co-authored-by: Aleksander Chlebowski <[email protected]> Signed-off-by: André Veríssimo <[email protected]>

[skip actions] Roxygen Man Pages Auto Update

3b9da92

pr: implements suggestions

71cfbf9

tests: rewrite tests to use testServer and adds splash

fc10479

tests: add expect_no_error to testServer calls

af76edc

gogonzo approved these changes Dec 5, 2023

View reviewed changes

tests/testthat/test-teal_data_module-eval_code.R Outdated Show resolved Hide resolved

R/teal_data_module-eval_code.R Outdated Show resolved Hide resolved

averissimo added 4 commits December 5, 2023 09:08

pr: remove lingering dontrun

d9fdbb9

pr: remove magrittr pipe usage

c012daf

lintr: corrects problem

f661743

pr: remove magrittr pipe usage and native

b1625ba

averissimo merged commit 32bfe75 into main Dec 5, 2023
24 checks passed

averissimo deleted the ddl@main branch December 5, 2023 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `within` and `eval_code` to `teal_data_module` #983

Adds `within` and `eval_code` to `teal_data_module` #983

averissimo commented Nov 28, 2023 •

edited

Loading

averissimo Nov 28, 2023 •

edited

Loading

chlebowa Nov 28, 2023

averissimo Nov 28, 2023

chlebowa Nov 28, 2023

chlebowa Nov 28, 2023

averissimo Nov 28, 2023

averissimo Nov 28, 2023

chlebowa Nov 28, 2023

chlebowa Dec 4, 2023

pawelru Dec 4, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

gogonzo left a comment

gogonzo left a comment

chlebowa Dec 4, 2023

chlebowa Dec 4, 2023

pawelru Dec 4, 2023

chlebowa Dec 4, 2023

gogonzo left a comment •

edited

Loading

Adds within and eval_code to teal_data_module #983

Adds within and eval_code to teal_data_module #983

Conversation

averissimo commented Nov 28, 2023 • edited Loading

Pull Request

Changes description

averissimo Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelru Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 28, 2023 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Nov 28, 2023 • edited Loading

Unit Tests Summary

github-actions bot commented Nov 28, 2023 • edited Loading

Unit Test Performance Difference

gogonzo left a comment

Choose a reason for hiding this comment

gogonzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gogonzo left a comment • edited Loading

Choose a reason for hiding this comment

Adds `within` and `eval_code` to `teal_data_module` #983

Adds `within` and `eval_code` to `teal_data_module` #983

averissimo commented Nov 28, 2023 •

edited

Loading

averissimo Nov 28, 2023 •

edited

Loading

pawelru Dec 4, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

github-actions bot commented Nov 28, 2023 •

edited

Loading

gogonzo left a comment •

edited

Loading